Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix UMAP issues with large inputs #6245

Open
wants to merge 30 commits into
base: branch-25.04
Choose a base branch
from
Open

Fix UMAP issues with large inputs #6245

wants to merge 30 commits into from

Conversation

viclafargue
Copy link
Contributor

Answers #6204

@viclafargue viclafargue requested a review from a team as a code owner January 22, 2025 13:29
@viclafargue viclafargue requested review from bdice and lowener January 22, 2025 13:29
@viclafargue viclafargue added bug Something isn't working non-breaking Non-breaking change labels Jan 22, 2025
@viclafargue viclafargue mentioned this pull request Jan 22, 2025
Open
wphicks
wphicks requested changes Jan 23, 2025
View reviewed changes
Copy link
Contributor

@wphicks wphicks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good! Let's just make sure to IWYU for the new uses of uint64_t. Using C++ types (std::uint64_t) in our non-CUDA code would be a bonus, but it shouldn't block merge. I've also called out some spots where we could use uniform initialization syntax rather than a bare cast.

cpp/include/cuml/common/callback.hpp Outdated Show resolved Hide resolved
cpp/include/cuml/manifold/common.hpp Show resolved Hide resolved
cpp/src/umap/fuzzy_simpl_set/naive.cuh Outdated Show resolved Hide resolved
cpp/src/tsne/tsne_runner.cuh Outdated Show resolved Hide resolved
cpp/src/umap/fuzzy_simpl_set/naive.cuh Outdated Show resolved Hide resolved
cpp/src/umap/knn_graph/algo.cuh Outdated Show resolved Hide resolved
cpp/src/umap/simpl_set_embed/algo.cuh Outdated Show resolved Hide resolved
@copy-pr-bot copy-pr-bot
Copy link

copy-pr-bot bot commented Jan 23, 2025

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@wphicks
Copy link
Contributor

wphicks commented Jan 23, 2025

@viclafargue That last commit was unsigned. Could you sign it and push that up?

cjnolet
cjnolet requested changes Jan 27, 2025
View reviewed changes
Copy link
Member

@cjnolet cjnolet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@viclafargue and I discussed this briefly last week, but given the nature of the 64-bit hardcoded changes here, I would like to see at least a small benchmark before this is merged so that we can feel comfortable that this doesn’t have a huge impact on the runtime.

@divyegala divyegala requested a review from a team as a code owner February 3, 2025 18:42
@divyegala divyegala requested a review from vyasr February 3, 2025 18:42
@github-actions github-actions bot added the CMake label Feb 3, 2025
@divyegala divyegala changed the base branch from branch-25.02 to branch-25.04 February 3, 2025 18:43
@divyegala divyegala requested a review from a team as a code owner February 3, 2025 18:43
@viclafargue viclafargue force-pushed the fix-umap-large-inputs branch from 288d129 to f7ce445 Compare February 5, 2025 18:43
@github-actions github-actions bot added the conda conda issue label Feb 5, 2025
wphicks
wphicks requested changes Feb 5, 2025
View reviewed changes
Copy link
Contributor

@wphicks wphicks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty good. The one change that I'm concerned about is with the blocks calculation. Let me know if I'm thinking about that one wrong.

cpp/cmake/thirdparty/get_cuvs.cmake Outdated Show resolved Hide resolved
cpp/src/tsne/tsne_runner.cuh Outdated Show resolved Hide resolved
cpp/src/umap/fuzzy_simpl_set/naive.cuh Show resolved Hide resolved
csadorf
csadorf reviewed Feb 5, 2025
View reviewed changes
dependencies.yaml Outdated Show resolved Hide resolved
cpp/cmake/thirdparty/get_cuvs.cmake Outdated Show resolved Hide resolved
cpp/cmake/thirdparty/get_raft.cmake Outdated Show resolved Hide resolved
cpp/cmake/thirdparty/get_treelite.cmake Outdated Show resolved Hide resolved
cpp/src/umap/simpl_set_embed/optimize_batch_kernel.cuh Outdated Show resolved Hide resolved
cpp/src/umap/runner.cuh Show resolved Hide resolved
cpp/src/umap/simpl_set_embed/algo.cuh Show resolved Hide resolved
cpp/src/umap/simpl_set_embed/runner.cuh Outdated Show resolved Hide resolved
cjnolet
cjnolet reviewed Feb 5, 2025
View reviewed changes
cpp/src/umap/runner.cuh Outdated Show resolved Hide resolved
cjnolet
cjnolet requested changes Feb 5, 2025
View reviewed changes
Copy link
Member

@cjnolet cjnolet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same issues as raft, but this is hardcoding types in perf critical code. Please use templates- it allows us to quickly switch.

@github-actions github-actions bot removed conda conda issue Cython / Python Cython or Python issue CMake labels Feb 7, 2025
@wphicks
Copy link
Contributor

wphicks commented Feb 7, 2025

/ok to test

@cjnolet
Copy link
Member

cjnolet commented Feb 7, 2025

/ok to test

csadorf
csadorf reviewed Feb 7, 2025
View reviewed changes
cpp/src/umap/simpl_set_embed/optimize_batch_kernel.cuh
T const* tail_embedding,
T* tail_buffer,
const MLCommon::FastIntDiv& tail_n,
int tail_n,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't be casting to int here, should we?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjnolet cjnolet cjnolet requested changes

@csadorf csadorf csadorf left review comments

@divyegala divyegala divyegala left review comments

@wphicks wphicks wphicks requested changes

@bdice bdice Awaiting requested review from bdice bdice is a code owner automatically assigned from rapidsai/cuml-cpp-codeowners

@lowener lowener Awaiting requested review from lowener lowener is a code owner automatically assigned from rapidsai/cuml-cpp-codeowners

@vyasr vyasr Awaiting requested review from vyasr vyasr was automatically assigned from rapidsai/cuml-cmake-codeowners

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
bug Something isn't working CUDA/C++ non-breaking Non-breaking change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@viclafargue @wphicks @divyegala @cjnolet @csadorf